etherGC.sol +reputationGC.sol #756
Conversation
|
Can you spell out "GC" in the contract name? No idea what it means... |
|
|
||
| /** | ||
| * @dev initialize | ||
| * @param _avatar the avatar to mint reputation from |
There was a problem hiding this comment.
this is a bit a confusing comment
| contract EtherGC { | ||
| using SafeMath for uint256; | ||
|
|
||
| uint256 public constraintPeriodWindow; //a static window |
There was a problem hiding this comment.
maybe call it "periodLength"? and add in the comment that the unit is blocks
| uint256 public constraintPeriodWindow; //a static window | ||
| uint256 public amountAllowedPerPeriod; | ||
| address public avatar; | ||
| uint256 public latestEthBalance; |
There was a problem hiding this comment.
Not sure what this is fore, this is just set in the initliaze call... if you need it, give it a more meaningful name perhaps?
| uint256 public amountAllowedPerPeriod; | ||
| address public avatar; | ||
| uint256 public latestEthBalance; | ||
| mapping(uint256=>uint256) public totalAmountSentForPeriods; |
There was a problem hiding this comment.
totalAmountSentPerPeriod, a mapping from period indexes to amounts
| uint256 public latestEthBalance; | ||
| mapping(uint256=>uint256) public totalAmountSentForPeriods; | ||
| uint256 public startBlock; | ||
| address public controller; |
There was a problem hiding this comment.
this needs some explanation, I think. So the constraint holds for what? it is a limit on how much the funds from the avatar the controller is allowed to spend per period.
| } | ||
|
|
||
| /** | ||
| * @dev check the allowance of ether sent per period. |
There was a problem hiding this comment.
"and throws an error if the constraint is violated"
test/etherGC.js
Outdated
|
|
||
| await setup(); | ||
| try { | ||
| await etherGC.initialize(avatar.address,20,web3.utils.toWei('5', "ether"),controller.address); //10 blocks ,5 eth |
There was a problem hiding this comment.
comment says 10 blocks, code says 20
| /** | ||
| * @title EtherGC ether constraint per period | ||
| */ | ||
| contract EtherGC { |
There was a problem hiding this comment.
you have a GlobalConstraint interface to inherit from?
| address _avatar, | ||
| uint256 _constraintPeriodWindow, | ||
| uint256 _amountAllowedPerPeriod, | ||
| address _controller |
There was a problem hiding this comment.
why do you need the controller as a separate argument, can't you get that from the avatar_?
test/etherGC.js
Outdated
| await controller.sendEther(web3.utils.toWei('4', "ether"), accounts[2],avatar.address); | ||
| //send more than 10 eth | ||
| await web3.eth.sendTransaction({from:accounts[0],to:avatar.address, value: web3.utils.toWei('10', "ether")}); | ||
| await controller.sendEther(web3.utils.toWei('4', "ether"), accounts[2],avatar.address); |
There was a problem hiding this comment.
I think you need to explain in a comment why this statement is not failing. Afaics, if this and the previous lines fall within the same period, you have sent 9 ether and violated the constraint. So by some coincidence, this last transfer happens in the next period?
| /** | ||
| * @dev initialize | ||
| * @param _avatar the avatar to enforce the constraint on | ||
| * @param _periodLength the periodLength in blocks units |
There was a problem hiding this comment.
I think period should not be in blocks, as block time is changing.
The timestamp cannot be trusted up to seconds, but it can be for large periods, as in this case.
| avatar = _avatar; | ||
| periodLength = _periodLength; | ||
| amountAllowedPerPeriod = _amountAllowedPerPeriod; | ||
| startBlock = block.number; |
There was a problem hiding this comment.
Start time/block is actually not really required, as you can pick any point in time to count from. But it makes the mapping more readable (start from index 0 instead of a random shift). Guessing it is worth the one-time 20k gas.
| using SafeMath for uint256; | ||
|
|
||
| uint256 public periodLength; //the period length in blocks units | ||
| uint256 public amountAllowedPerPeriod; |
There was a problem hiding this comment.
Not sure if this should be percentage-wise, or absolute values. Probably better to keep it absolute.
| totalAmountSentPerPeriod[currentPeriodIndex] = | ||
| totalAmountSentPerPeriod[currentPeriodIndex].add(avatarBalanceBefore.sub(address(avatar).balance)); | ||
| require(totalAmountSentPerPeriod[currentPeriodIndex] <= amountAllowedPerPeriod, | ||
| "Violation of Global constraint EtherGC:amount sent exceed in current period"); |
There was a problem hiding this comment.
Should set back avatarBalanceBefore to zero, as it will save gas.
| contract ReputationGC is GlobalConstraintInterface { | ||
| using SafeMath for uint256; | ||
|
|
||
| uint256 public periodLength; //the period length in blocks units |
No description provided.